-
Notifications
You must be signed in to change notification settings - Fork 261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🪛 Moving to SQLAlchemy 2.0 #540
Conversation
* Removed support for python 3.7 * Added common and dialects packages to handle the new SQLAlchemy 2.0+
* The connections were hanging sometimes during the tests.
@aminalaee @Kludex maybe you could manually kill:
Please? This was the whole reason why the second commit. Quite a well known issue but that CI passed with flying colours, only these 2 are hanging |
Done |
Well, I hope this is ok. Quite nice and clean integration for SQLAlchemy 2.0 |
I've forked the version of this PR and it's working fine in our apps, waiting for the release 😄 |
@lucasguiss very happy to hear that :) |
@Kludex here the status is not "complete" because this PR also makes sure python 3.7 is no longer supported for many reasons but also in June the support will stop. |
Why did you remove now? |
Well, I can always add and we remove after june, I would say. The initial tests were failing with some 3.7 issues but I will add it back. I think I solved them |
Can we remove all changes that are not related to the support of SQLAlchemy 2.0? You can create other PRs for them, but let's focus on a single thing here, please. |
@Kludex Done. It was mainly that, had initial configuration issues with python 3.7 but that was addressed. The changes you see are the ones used for the move to SQLAlchemy 2.0+ Regarding other PRs. I can do that later once the time comes. This was already a good change that allow us to move forward. |
* Revert changes automatically applied by IDE * Fix formatting of CI caused by IDE changes * Codebase cleanup for SQLAlchemy 2.0 * Fix EOL in mkdocs
23223c9
to
b271a54
Compare
b271a54
to
deedd13
Compare
Hey @aminalaee . Any news about this? I know you are extremely busy |
Hi @tarsil , I will take a look today or tomorrow. Sorry for the delay. |
I think you'll have to update Line 50 in b6eba5f
|
I did update it as well before. Its there 👍🏼 |
pip complained when I installed the branch, but I see it now. Not sure why I missed it before - sorry for the noise. |
@aminalaee sorry to a bother but, any news? |
@Kludex this ia probably my fault. Since our last conversation unfortunately I didn't have enough time to address these comments. @rafalp in your code comment. I know SQLA stopped supporting that and that is the reason why I didn't change too much and I simple added the * as per normal python. I promise I will try to address all the comments soon. Apologies |
Few days ago SQLAlchemy released 2.0.11 where they have changed |
return raw | ||
|
||
def __iter__(self) -> typing.Iterator: | ||
return iter(self._row.keys()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of this you can't unpack result anymore
Example:
class LastUsername(Base):
__tablename__ = 'last_username'
user_id = Column(BIGINT, primary_key=True)
username = Column(TEXT, nullable=False)
r = await db.fetch_one(select(LastUsername))
user_id, username = r
assert user_id == 'user_id' #but it need to be at least int
hum, any hope for this to be merged ? |
If comments are addressed or answered, we can continue here |
Honestly I just decided to give up on this library altogether and use the new async features built-in into SA2. |
Any update? |
1 similar comment
Any update? |
Sorry, didn't have enough time to fully address this but I will soon |
@tarsil -- We'll be kicking the tires more on this over the coming weeks and sharing any feedback or discoveries here. |
Any updates or roadmap for this PR? |
We just shipped this branch out to production and haven't noticed any problems yet. |
I've tried out this branch by specifying the following in my requirements file:
which resulted in pip-compile choosing As previously mentioned here the Row class has changed in v2.0.11 resulting in an error like this when performing a query:
I think it would be worth updating this PR to account for the new Row class before merging, otherwise projects using @tarsil I'm not very familiar with SqlAlchemy internals but the solution would be something like this: adnam@96e15fd |
Fix compatibility with SQLAlchemy>=2.0.11
When @tarsil is happy (and this is ready to be merged...), I'll merge it and make a release. Ping me. |
@Kludex i do believe I just need to fix the conflicts and we should be good to go |
I've invited you to encode @tarsil . Ask me on PM if you have questions about workflows or anything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge when happy @tarsil 👍
@Kludex , I just merged after fixing the minor conflicts (normal as it was an old branch) and passed 👍🏼 |
Will do but so far was just this simple merge conflict, I believe. Thank you for the invite. |
@aminalaee @Kludex this is a cleaner version of the previous PR with the changes passing the new SQLAlchemy 2.0 which is significantly different from the < 2.0
It is also good to mention that when I was doing this PR, I was reading some suggestions and the one from here #507 was good to add it as well. So I would like to thank @circlingthesun and @JGoutin for the suggestions
Edit: The asyncpg sometimes hangs during the tests. I forcibly close the connections during that test to make sure it doesn't get stuck